Fix keysend not being sent in OutboundPayment::send_payment_internal#2475
Conversation
Fixes a bug where we wouldn't use the provided keysend preimage when piping through OutboundPayment::pay_route_internal. Also simplifies and refactors existing keysend tests to make sure this gets hit.
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2475 +/- ##
==========================================
- Coverage 90.34% 90.34% -0.01%
==========================================
Files 106 106
Lines 55784 55771 -13
Branches 55784 55771 -13
==========================================
- Hits 50398 50386 -12
+ Misses 5386 5385 -1
☔ View full report in Codecov by Sentry. |
| nodes[0].logger, &scorer, &(), &random_seed_bytes | ||
| ).unwrap(); | ||
|
|
||
| let test_preimage = PaymentPreimage([42; 32]); |
There was a problem hiding this comment.
The real issue is that the test just reused the preimage instead of extracting it from the event. Can you put the preimage and send in a {}, that way the claiming logic further down has no access to it at all.
There was a problem hiding this comment.
I think that's okay since pass_along_path checks that the preimage from Event::PaymentClaimable matches the expected one passed in?
I had mainly noticed that a problem was that we never tested a full payment using spontaneous_with_retry which uses OutboundPayment::send_payment_internal whereas the method without retries doesn't
There was a problem hiding this comment.
Not really a big deal, I wrote up more of what I was suggesting as #2481, which I'm fine with not merging if anyone disagrees, but I like the explicit scope to demonstrate information lifetime.
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
LGTM, new test fails when I revert the change. Not the first time we've unknowingly broken keysend :( We have existing coverage of keysend retrying when initiated with send_spontaneous_payment_with_retry, but were missing first-try-success coverage.
I don't quite get @TheBlueMatt's comment but it sounds like it can be addressed in follow-up so I'll merge
5 participants
Following up on #2465. I kept thinking that it doesn't make sense that we'd reject a keysend payment based on our feature bit not being set. After some digging, I found the issue was that we weren't sending the preimage.